Skip to content

[sw,dv] Add SW-DV logging#600

Open
martin-velay wants to merge 4 commits into
lowRISC:mainfrom
martin-velay:sw_dv_log
Open

[sw,dv] Add SW-DV logging#600
martin-velay wants to merge 4 commits into
lowRISC:mainfrom
martin-velay:sw_dv_log

Conversation

@martin-velay

Copy link
Copy Markdown
Contributor

Related to this issue: #261

@rswarbrick rswarbrick left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nitty comments but this looks nice.

(And the review took me a while: I hadn't seen the clever variadic macro trick before!)

Comment thread hw/top_chip/dv/env/seq_lib/top_chip_dv_dv_log_smoketest_vseq.sv Outdated
Comment thread hw/top_chip/dv/env/seq_lib/top_chip_dv_dv_log_smoketest_vseq.sv Outdated
Comment thread hw/top_chip/dv/env/seq_lib/top_chip_dv_dv_log_smoketest_vseq.sv Outdated
Comment thread hw/top_chip/dv/env/top_chip_dv_env_pkg.sv Outdated
Comment thread hw/vendor/lowrisc_ip/util/device_sw_utils/extract_sw_logs.py Outdated
@martin-velay

Copy link
Copy Markdown
Contributor Author

I've just rebased and fix the merge conflicts. There was also an issue for CHERI tests. Now Verilator tests pass and the UVM tests too (new tests dv_log_smoketest and dv_log_smoketest_cheri in test_framework regression):

dvsim hw/top_chip/dv/top_chip_sim_cfg.hjson -i test_framework
...
          [   legend    ]: [S: scheduled, Q: queued, R: running, P: passed, F: failed, K: killed, T: total]                                                                                               
00:01:07  [    build    ]: [S:         0, Q:      0, R:       0, P:      1, F:      0, K:      0, T:     1] 100%                                                                                          
00:01:43  [     run     ]: [S:         0, Q:      0, R:       0, P:      6, F:      0, K:      0, T:     6] 100%  
...

And the log gives:

UVM_INFO @            553397000: (sw_logger_if.sv:524) [dv_log_smoketest_vanilla_sram(sw/device/tests/test_framework/dv_log_smoketest.c:14)] SW-DV log smoketest starting...
UVM_INFO @            560717000: (sw_logger_if.sv:524) [dv_log_smoketest_vanilla_sram(sw/device/tests/test_framework/dv_log_smoketest.c:15)] a = 10, b = 20, c = 12
UVM_INFO @            565357000: (sw_logger_if.sv:524) [dv_log_smoketest_vanilla_sram(sw/device/tests/test_framework/dv_log_smoketest.c:16)] a + b + c = 42
UVM_INFO @            568337000: (sw_logger_if.sv:524) [dv_log_smoketest_vanilla_sram(sw/device/tests/test_framework/dv_log_smoketest.c:17)] Smoke test for the SW-DV log interface, completed successfully!

@rswarbrick rswarbrick left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good to me.

Thanks for the updates (and for teaching me about variadic macros in C...)

@marnovandermaas marnovandermaas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments from my end.

Comment thread hw/top_chip/dv/env/seq_lib/top_chip_dv_dv_log_smoketest_vseq.sv
Comment thread sw/device/lib/boot/mocha.ld
Comment thread sw/device/lib/test_framework/dv_log.h

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@engdoreis what do you think of this? Are we reinventing the wheel here?

Comment thread sw/device/lib/test_framework/dv_log.c Outdated
va_list args;
va_start(args, log);
for (uint32_t i = 0; i < log->nargs; i++) {
DEV_WRITE(&dv_window->log, va_arg(args, uint32_t));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this 32-bit and not 64-bit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DV monitor consumes 32-bit beats, a 64 bits value would be truncated with today's implementation

Comment thread sw/device/lib/test_framework/dv_log.c Outdated
void dv_log_write(const log_fields_t *log, ...)
{
dv_window_t dv_window = mocha_system_dv_window();
DEV_WRITE(&dv_window->log, (uint32_t)(uintptr_t)log);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointers are 64-bit right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sim SRAM window is only 32-bits and the sw_logger_if only samples 32-bits and the upper bits have an assertion in the tb.sv to ensure it's always the case

Comment thread util/build_sw_collateral_for_sim.py Outdated
Comment thread util/build_sw_collateral_for_sim.py

DV_LOG_INFO("SW-DV log smoketest starting...");
DV_LOG_INFO("a = %d, b = %d, c = %d", 10, 20, 12);
DV_LOG_INFO("a + b + c = %d", 42);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to also test %x, %c, %s?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also test priting a capability since you added support for it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea on %x/%c/%s - I'll add a line exercising them to the smoketest.

On printing a capability: the log arg path only carries 32-bit values (dv_log_write reads each arg as va_arg(uint32_t)), so we can log a capability's address via %x/%p but not the full 128-bit capability. The CHERI support in this PR is about extracting the log database from a CHERI-built ELF and writing the log port safely (VOLATILE_WRITE), both already exercised end-to-end by the dv_log_smoketest_cheri variant. Did you mean logging an address, or were you thinking of the DB extraction that the _cheri test already covers? Full 128-bit capability logging would need a wider arg path, which I'd suggest as a follow-up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@engdoreis, I did a test actually and it seems that the %c isn't cleanly supported by the upstream cleanup: it rewrites %c to %0c (the same way %d becomes %0d), and SV $sformatf rejects %0c (*W,SYSFMW: Illegal format ignored), so the log warns even though the char still prints. %x/%s are fine. I can either drop %c from the smoketest or fix cleanup_format to leave %c untouched (one-token change, and %0c is invalid SV anyway). Any preference?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now let's just test the supported formats.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall I open an issue to add this later?

Comment thread sw/device/lib/test_framework/dv_log.h Outdated
Comment thread sw/device/lib/test_framework/dv_log.h
Comment thread sw/device/lib/test_framework/dv_log.c Outdated
The upstream script assumes a 32-bit struct layout (5 x uint32, 20 B,
LONG header). Mocha's rv64 log_fields_t uses 8-byte const char * pointer
fields, giving a 32-byte struct and an 8-byte QUAD section-address
header. Under CHERI those pointers are 16-byte capabilities, forcing
16-byte section alignment, a 16-byte header and 64-byte entries, with
the file/format addresses held in __cap_relocs rather than the section.

Add a patch that selects the layout from the ELF class and the
.logs.fields alignment, and resolves capability pointer fields from
__cap_relocs when present.

Wire the script into build_sw_collateral_for_sim.py so each CMake
install step produces the .logs.txt and .rodata.txt files that
sw_logger_if needs at simulation start.

Signed-off-by: martin-velay <mvelay@lowrisc.org>
Each call site places a log_fields_t entry in a new .logs.fields ELF
section; extract_sw_logs.py produces the database consumed by
sw_logger_if to decode printf-style messages at simulation start, with
no UART overhead.

Add a log field at offset 0x8 of the dv_window_memory_layout struct and
route dv_log_write() through mocha_system_dv_window() for CHERI-safe
access to the log write port (0x2002_0008). Add the .logs.fields section
to the linker script after .rodata so string constants are resolved
before log entries are laid out. Add dv_log.h/c implementing the
DV_LOG_INFO/WARNING/ERROR/FATAL macros and the runtime dv_log_write()
function. Add dv_log_smoketest to verify the end-to-end path.

Signed-off-by: martin-velay <mvelay@lowrisc.org>
Signed-off-by: martin-velay <mvelay@lowrisc.org>
Signed-off-by: martin-velay <mvelay@lowrisc.org>

@engdoreis engdoreis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's wait for Marno's approval before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants